Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

UI/importers: Translate capture sources depending on display server #11412

Merged
merged 1 commit into from
Oct 30, 2024
Merged

UI/importers: Translate capture sources depending on display server #11412

merged 1 commit into from
Oct 30, 2024

Conversation

SarenDev
Copy link
Contributor

@SarenDev SarenDev commented Oct 18, 2024 •
edited
Loading

Description

Fixes a crash when importing scenes from Windows by translating window and game capture sources to xcomposite or pipewire depending on the display server used.
This change prevents xcomposite from being assigned on non-X11 systems, which causes the crash due to the module not being loaded.

Motivation and Context

Fixes #5323

How Has This Been Tested?

Tested under Ubuntu 24.10 with KDE Plasma 6.1.5 under both Wayland and X11

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@WizardCM WizardCM added Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality Linux Affects Linux labels Oct 18, 2024
@SarenDev
Copy link
Contributor Author

I also noticed that the Windows and MacOS translation implementations are missing translations for pipewire. Should they be added to this request or a new one?

tytan652
tytan652 suggested changes Oct 18, 2024
View reviewed changes
Copy link
Collaborator

@tytan652 tytan652 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Translate capture sources depending on WM

This is not a good wording, Wayland and X11 are not WMs but windowing systems.

UI/importers/studio.cpp Show resolved Hide resolved
UI/importers/studio.cpp Outdated Show resolved Hide resolved
@SarenDev
Copy link
Contributor Author

Translate capture sources depending on WM

This is not a good wording, Wayland and X11 are not WMs but windowing systems.

Sorry, I realized that after I submitted my request

@SarenDev SarenDev changed the title UI/importers: Translate capture sources depending on WM UI/importers: Translate capture sources depending on display server Oct 18, 2024
@norihiro
Copy link
Contributor

The commit d7b1be3 has a too long description in a line, 88 characters, limit 72:

Use pipewire-screen-capture instead of the currently discouraged pipewire-window-capture

@SarenDev SarenDev force-pushed the window-capture-translation-wayland-fix branch from d7b1be3 to 42429d6 Compare October 19, 2024 10:16
@SarenDev
Copy link
Contributor Author

The commit d7b1be3 has a too long description in a line, 88 characters, limit 72:

Use pipewire-screen-capture instead of the currently discouraged pipewire-window-capture

Corrected, apologies

@SarenDev SarenDev force-pushed the window-capture-translation-wayland-fix branch from 42429d6 to dbc1ae1 Compare October 19, 2024 10:23
@norihiro
Copy link
Contributor

The folding looks fine. Though, three commits should be squashed into one commit.

@Lain-B
Copy link
Collaborator

Lain-B commented Oct 26, 2024

Agreed with norihiro, squash the latter two commits into the first commit and it should be good.

@SarenDev SarenDev force-pushed the window-capture-translation-wayland-fix branch from dbc1ae1 to 9bcb583 Compare October 26, 2024 22:02
@SarenDev
UI/importers: Translate capture sources depending on WS
d030983 
Translate window or game capture sources to xcomposite or pipewire
depending on the window system used.
This change prevents xcomposite being assigned on non-X11 systems
which causes a crash
@SarenDev SarenDev force-pushed the window-capture-translation-wayland-fix branch from 9bcb583 to d030983 Compare October 26, 2024 22:07
@RytoEX
Copy link
Member

RytoEX commented Oct 29, 2024

If a user imports a scene collection in Wayland, and then swaps to X11 later, what happens to these sources? Do they simply not load? Will OBS crash if you try to access their properties?

cc @kkartaltepe since we might have had prior conversations about what to do about this.

If this fixes a GitHub issue, use this syntax in the PR description:

Fixes #5323

@kkartaltepe
Copy link
Collaborator

If a user imports a scene collection in Wayland, and then swaps to X11 later, what happens to these sources? Do they simply not load? Will OBS crash if you try to access their properties?

They just have no properties and are not useful. This was considered fine when we decided to split the capture plugins between the platforms. I don't have a problem with the implementation as is.

@RytoEX RytoEX merged commit 23e772c into obsproject:master Oct 30, 2024
14 checks passed
@RytoEX RytoEX added this to the OBS Studio 31 milestone Oct 30, 2024
@SarenDev SarenDev deleted the window-capture-translation-wayland-fix branch October 30, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tytan652 tytan652 tytan652 requested changes

@RytoEX RytoEX RytoEX approved these changes

@Lain-B Lain-B Lain-B approved these changes

Assignees
No one assigned
Labels
Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality Linux Affects Linux
Projects
None yet
Milestone
OBS Studio 31
7 participants
@SarenDev @norihiro @Lain-B @RytoEX @kkartaltepe @tytan652 @WizardCM